feat(Portal): More flexible configuration#590
Conversation
jeffcarbs
force-pushed
the
feature/portal-config
branch
from
50b2cbc to
77e7fed
Compare
Current coverage is 100% (diff: 100%)@@ master #590 diff @@
====================================
Files 119 119
Lines 1894 1915 +21
Methods 0 0
Messages 0 0
Branches 0 0
====================================
+ Hits 1894 1915 +21
Misses 0 0
Partials 0 0
|
|
@levithomason - this is blocking the |
|
On it, thanks! |
|
@jcarbo, I pulled your branch into the Popup and noticed that the first time the Popup shows up the timer on leave isn't respected. The bug goes away every time I hover the trigger after that. |
src/addons/Portal/Portal.js
Outdated
|
|
||
| /** Controls whether or not the portal should close when mousing out of the trigger. */ | ||
| closeOnTriggerMouseLeave: PropTypes.bool, | ||
| /** Controls whether or not the portal should close on blur of the trigger. */ |
There was a problem hiding this comment.
close on blur != closeOnTriggerClick
src/addons/Portal/Portal.js
Outdated
| * Controls whether or not the portal should close when mousing out of the | ||
| * trigger OR the portal content. | ||
| */ | ||
| closeOnMouseLeave: PropTypes.bool, |
There was a problem hiding this comment.
Hm, seems like we should separate these. I know the names are hideous, but I can see wanting one or the other:
closeOnTriggerMouseLeave
closeOnPortalMouseLeave
Thoughts?
There was a problem hiding this comment.
That was my initial thought, but then I was wondering how do you configure allowing mousing from the trigger to the portal? I think I just figured it out though:
- If only
closeOnTriggerMouseLeaveis set, then mouseover/leave on the portal won't do anything - If
closeOnPortalMouseLeaveis set, then mouseover of the portal clears the timer, mouseleave restarts the timer.
So closeOnPortalMouseLeave has the added effect of preventing close when mousing over the gap from trigger -> portal
This actually lets us keep more closely with SUI's hoverable:
hoverable => falseis{ closeOnTriggerMouseLeave: true }hoverable => trueis{ closeOnTriggerMouseLeave: true, closeOnPortalMouseLeave: true }
There was a problem hiding this comment.
I'm moving quickly here, but I think this makes sense.
|
|
||
| closeWithTimeout = (e, delay) => { | ||
| // React wipes certain props (e.g. currentTarget) so we need to clone. | ||
| const eventClone = { ...e } |
There was a problem hiding this comment.
Heads up, React wipes the entire event object, nullifying all values. That's because React reuses a single event object. In order to persist the event object for async access, use e.persist():
https://facebook.github.io/react/docs/events.html#event-pooling
I think what is here will technically work, but it still seems as though we should be using e.persist(), even if we're copying the persisted object.
There was a problem hiding this comment.
Tried e.persist initially but doesn't preserve the currentTarget, see facebook/react#2857. This is kinda gross but it's the only way to make a copy of the event as-is.
There was a problem hiding this comment.
Womp womp, thanks for the link
|
@jcarbo Nevermind the issue I reported earlier (aka. timer not respected the 1st time), I can't reproduce it anymore. |
* Add closeOnTriggerClick prop * Update closeOnMouseLeave to handle mouse leave of either trigger or portal * Add configurable delay for open/close on hover
jeffcarbs
force-pushed
the
feature/portal-config
branch
from
1a5763f to
f65de27
Compare
|
Ok made the changes discussed here and checks came back green. I think this is 👍 |
|
Released in |
4 participants
closeOnTriggerClickpropcloseOnMouseLeaveto handle mouse leave of either trigger or portalThis addresses an open issue of #570 which is not being able to mouse-over from the trigger of a
Popup.